Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP authentication, API key and some CLI improvements #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vithyze
Copy link
Contributor

@vithyze vithyze commented Mar 5, 2023

The LDAP authentication works by:

Look up the user in the LDAP server
If the user exists try to authenticate (bind)
If it succeeds and the user doesn't exist in the database a new one is created (with ldap=1)
If the user does not exist try the supysonic database (with ldap=0)

Other things considered:

It requires the ldap3 module but if it's not installed it just throws a warning in the log.
To disable LDAP authentication just comment the ldap section in the config.
The mysql fields "password" and "salt" are now nullable because they are null if the user is from LDAP.
A new mysql field was added "ldap" with default 0.
It adds an icon to the user profile if they are a LDAP user.
Some features like changing email and password are disabled because it doesn't make sense to change them manually. Changing username is allowed.

Regarding the API key, I think its safer to use a key for API authentication since it is just sent encoded and not encrypted, so since I was modifying the code for LDAP authentication I also joined these changes because it would then be too confusing.

The API key authentication works by:

If require_api_key is enabled: try to login with the API key
If require_api_key is disabled: try API key first then password if it fails

It adds a form to the user profile to generate a new key or delete the existing one.
It adds new option group to the cli: "user api key" with commands: show, new and delete
A new mysql field was added "api_key" with default 0.

I also adapted the cli to the LDAP authentication and ended up making some improvements:

Added a new option group "user edit"
Moved "changepass" to "user edit password"
Moved "rename" to "user edit username"
Added a new option "user edit email"

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Merging #250 (b2f0c99) into master (8e2adf8) will decrease coverage by 24.38%.
The diff coverage is 49.37%.

@@             Coverage Diff             @@
##           master     #250       +/-   ##
===========================================
- Coverage   86.41%   62.04%   -24.38%     
===========================================
  Files          46       47        +1     
  Lines        3776     3915      +139     
===========================================
- Hits         3263     2429      -834     
- Misses        513     1486      +973     
Impacted Files Coverage Δ
supysonic/cli.py 71.13% <38.33%> (-19.68%) ⬇️
supysonic/frontend/user.py 93.30% <50.00%> (-6.70%) ⬇️
supysonic/ldap.py 50.00% <50.00%> (ø)
supysonic/managers/user.py 82.50% <56.00%> (-17.50%) ⬇️
supysonic/api/__init__.py 57.94% <75.00%> (-39.09%) ⬇️
supysonic/config.py 95.83% <100.00%> (+0.08%) ⬆️
supysonic/db.py 86.36% <100.00%> (-6.28%) ⬇️
supysonic/api/albums_songs.py 14.78% <0.00%> (-84.51%) ⬇️
supysonic/api/search.py 15.58% <0.00%> (-84.42%) ⬇️
supysonic/api/annotation.py 16.37% <0.00%> (-83.63%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@spl0k
Copy link
Owner

spl0k commented Mar 5, 2023

Hello again.

What do you mean by API key? Please add description to your PR so I know what I'm reviewing. Actually if you're adding several new authentication methods, could you please split this PR (a PR per auth method)? And give a proper title to your commits please, once merged we'll only have an "Apply changes" in the history and that makes it hard to browse later (especially when building changelogs for releases).

I haven't reviewed the code yet, but just looking at the list of changed files I can tell than this PR is missing:

  • migrations
  • documentation
  • and some tests would be nice too, especially if we're talking about authentication

@vithyze
Copy link
Contributor Author

vithyze commented Mar 5, 2023

Hi, thanks for the quick response. Also thanks for your software I tried many servers but yours have been working great for some years now. Indeed I should have added more details, I edited the original post. The commit history is just "Apply changes" because I merged a patch from a local repository (sorry I'm a bit noob to git lol). I have been using these changes (and the others from the other branches) for almost an year and it has been fine. Regarding the missing stuff I can have a look at what's missing from the documentation but if you could help me with the migrations/tests or add those later on it would be nice. Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants